Add an AbiDecode library for safe abi decoding (no revert)#6393
Add an AbiDecode library for safe abi decoding (no revert)#6393Amxx wants to merge 4 commits intoOpenZeppelin:masterfrom
Conversation
|
This library seems like a good idea but, how far do we want to go with it? Decoding more complex values still requires the developer to know how to read offsets and validate bounds for the outer structure. The library might end up too large I have a bunch of calldata decoding cases in OAF that are very extensive and this lib would definitely help |
🦋 Changeset detectedLatest commit: 0d667c9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThis pull request introduces a new 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/utils/AbiDecode.t.sol`:
- Around line 34-40: The success branch in testDecodeCalldataNoRevert is
asserting empty bytes incorrectly; when tryDecodeBytesCalldata returns success
== true you should verify the decoded output matches the original input buffer.
Update the success branch assertion in testDecodeCalldataNoRevert to compare
output to the original buffer (e.g., assertEq(output, bytes(buffer)) or
equivalently abi.decode(buffer, (bytes))) so that tryDecodeBytesCalldata,
success, and output are validated correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6eedfff9-aab4-4037-87b4-7003e47271c4
📒 Files selected for processing (3)
contracts/utils/AbiDecode.solcontracts/utils/Memory.soltest/utils/AbiDecode.t.sol
| function testDecodeCalldataNoRevert(bytes calldata buffer) public pure { | ||
| (bool success, bytes calldata output) = buffer.tryDecodeBytesCalldata(); | ||
| if (success) { | ||
| assertEq(output, new bytes(0)); | ||
| } else { | ||
| assertEq(output, new bytes(0)); | ||
| } |
There was a problem hiding this comment.
Fix the success-branch assertion in testDecodeCalldataNoRevert.
Line 37 currently asserts empty bytes even when success == true, so the success path is not actually verifying decoded content.
✅ Suggested test fix
function testDecodeCalldataNoRevert(bytes calldata buffer) public pure {
(bool success, bytes calldata output) = buffer.tryDecodeBytesCalldata();
if (success) {
- assertEq(output, new bytes(0));
+ assertEq(output, abi.decode(buffer, (bytes)));
} else {
assertEq(output, new bytes(0));
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/utils/AbiDecode.t.sol` around lines 34 - 40, The success branch in
testDecodeCalldataNoRevert is asserting empty bytes incorrectly; when
tryDecodeBytesCalldata returns success == true you should verify the decoded
output matches the original input buffer. Update the success branch assertion in
testDecodeCalldataNoRevert to compare output to the original buffer (e.g.,
assertEq(output, bytes(buffer)) or equivalently abi.decode(buffer, (bytes))) so
that tryDecodeBytesCalldata, success, and output are validated correctly.
I'd keep it simple and small (kiss approach), only adding things that we internally need, or that we know would be reusable. The goal is not to support all types from the start. We'll just add stuff as we see fit. |
2 participants
Would be usefull for #6390
PR Checklist
npx changeset add)